Skip to content

Disable accounts.in_memory_only by default#8445

Open
ripatel-fd wants to merge 4 commits intofiredancer-io:mainfrom
riptl:ripatel/vinyl-default
Open

Disable accounts.in_memory_only by default#8445
ripatel-fd wants to merge 4 commits intofiredancer-io:mainfrom
riptl:ripatel/vinyl-default

Conversation

@ripatel-fd
Copy link
Contributor

No description provided.

Copilot AI review requested due to automatic review settings February 24, 2026 23:44
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR changes the default behavior of the accounts database configuration from in-memory-only mode to using persistent storage. The primary change is setting accounts.in_memory_only = false in the default configuration file, while refactoring test scripts to explicitly specify in-memory mode when needed.

Changes:

  • Modified default configuration to disable accounts.in_memory_only (changed from true to false)
  • Refactored test scripts to replace --vinyl flag with --in-memory-only flag
  • Updated test scripts to explicitly enable in-memory mode where needed

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/app/firedancer/config/default.toml Changed default value of in_memory_only from true to false
src/flamenco/runtime/tests/run_ledger_backtest.sh Replaced --vinyl flag with --in-memory-only and --accounts-db flags, restructured config generation logic
src/flamenco/runtime/tests/run_backtest_ci.sh Replaced --vinyl flag with --in-memory-only in test invocations, removed outdated comments
src/flamenco/runtime/tests/run_backtest_all.sh Replaced --vinyl flag with --in-memory-only in test invocations, added new test cases with explicit in-memory mode
contrib/test/test_firedancer_leader.sh Added explicit in_memory_only = true to maintain existing behavior
contrib/test/run_solcap_tests.sh Added explicit in_memory_only = true to maintain existing behavior
contrib/test/run_fd_shred_cap.sh Added explicit in_memory_only = true to maintain existing behavior
contrib/offline-replay/offline_replay.toml Added explicit in_memory_only = true to maintain existing behavior
opt Added symlink to ../firedancer/opt

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@ripatel-fd ripatel-fd force-pushed the ripatel/vinyl-default branch from f37a2a1 to 152eda4 Compare February 24, 2026 23:47
Copilot AI review requested due to automatic review settings February 25, 2026 01:28
@ripatel-fd ripatel-fd force-pushed the ripatel/vinyl-default branch from 152eda4 to 2e75d68 Compare February 25, 2026 01:28
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@ripatel-fd ripatel-fd force-pushed the ripatel/vinyl-default branch from 2e75d68 to fc93a17 Compare February 27, 2026 21:46
Copilot AI review requested due to automatic review settings February 27, 2026 21:49
@ripatel-fd ripatel-fd force-pushed the ripatel/vinyl-default branch from fc93a17 to 8548663 Compare February 27, 2026 21:49
kbhargava-jump
kbhargava-jump previously approved these changes Feb 27, 2026
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot AI review requested due to automatic review settings February 28, 2026 18:36
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 7 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

# If enabled, allocates [accounts.file_size_gib] memory and ignores
# [accounts.{cache_size_gib,max_unrooted_account_size_gib}].
in_memory_only = true
in_memory_only = false
Copy link

Copilot AI Feb 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switching accounts.in_memory_only to false changes the default storage mode to on-disk accounts and also makes [accounts.cache_size_gib,max_unrooted_account_size_gib] mandatory. Given the new very large file_size_gib default above, this can cause unexpected disk preallocation for configs that rely on defaults (e.g. devnet). If the intent is only to flip the default for mainnet, consider overriding in_memory_only=false in mainnet configs instead of changing the global default.

Suggested change
in_memory_only = false
in_memory_only = true

Copilot uses AI. Check for mistakes.
Comment on lines 64 to 68
local rcpath; rcpath="$(rc_path "$pid")"
{
echo "$ret"
echo "$elapsed"
} > "$rcpath"
Copy link

Copilot AI Feb 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

runner() writes results to rc_path, but the rc_path() helper was removed and is no longer defined anywhere in this script. With set -euo pipefail, this will fail with rc_path: command not found after the first test run. Either reintroduce rc_path() (and any associated RC handling) or remove the rc file write logic now that tests are run synchronously.

Copilot uses AI. Check for mistakes.
Comment on lines +94 to +95
dispatch "$OBJDIR"/unit-test/test_fddev
dispatch "$OBJDIR"/unit-test/test_firedancer_dev --testnet
Copy link

Copilot AI Feb 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dispatch() is invoking binaries under $OBJDIR/unit-test/..., but test_fddev and test_firedancer_dev are built via make-integration-test, which places them under $OBJDIR/integration-test/.... As written, dispatch will skip running the tests entirely (due to the -f check). Update the paths to the integration-test directory (or change the build rules to emit these under unit-test).

Suggested change
dispatch "$OBJDIR"/unit-test/test_fddev
dispatch "$OBJDIR"/unit-test/test_firedancer_dev --testnet
dispatch "$OBJDIR"/integration-test/test_fddev
dispatch "$OBJDIR"/integration-test/test_firedancer_dev --testnet

Copilot uses AI. Check for mistakes.
shift 1

if [[ ! -f "$prog" ]]; then
return 0
Copy link

Copilot AI Feb 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dispatch() returns success when the target binary does not exist. This can mask misconfigurations (or path bugs) and produce false-positive test runs. Consider failing with a clear error message when an expected test binary is missing, or at least emitting a warning and returning non-zero in CI.

Suggested change
return 0
echo "test.sh: expected test binary not found: $prog" >&2
return 1

Copilot uses AI. Check for mistakes.
Comment on lines 268 to 271
make-unit-test = $(eval $(call _make-exe,$(1),$(2),$(3),unit-test,unit-test,$(4) $(LDFLAGS_EXE)))
run-unit-test = $(eval $(call _run-unit-test,$(1)))
make-integration-test = $(eval $(call _make-exe,$(1),$(2),$(3),integration-test,integration-test,$(4) $(LDFLAGS_EXE)))
run-integration-test = $(eval $(call _run-integration-test,$(1)))
make-fuzz-test = $(eval $(call _fuzz-test,$(1),$(2),$(3),$(4) $(LDFLAGS_EXE)))
Copy link

Copilot AI Feb 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

run-integration-test macro was removed here, but the earlier usage/documentation block in this file still advertises $(call run-integration-test,name,args). To avoid confusion and broken expectations when adding tests, update the documentation to remove/replace that usage or reintroduce a supported macro.

Copilot uses AI. Check for mistakes.
@@ -232,17 +232,6 @@ $(OBJDIR)/unit-test/automatic.txt: $(LOCAL_MKS)
$(RM) $@
@$(foreach test,$(RUN_UNIT_TEST),echo $(test)>>$@;)

Copy link

Copilot AI Feb 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rule that generated $(OBJDIR)/integration-test/automatic.txt was removed, but make run-integration-test still passes --tests $(OBJDIR)/integration-test/automatic.txt (see this file earlier). This makes the integration-test runner dependent on a file that is no longer produced anywhere. Either restore generation of the integration test list, or update the run target/runner to not reference integration-test/automatic.txt.

Suggested change
# Generate list of automatic integration tests from $(call run-integration-test,...)
integration-test: $(OBJDIR)/integration-test/automatic.txt
define _run-integration-test
RUN_INTEGRATION_TEST+=$(OBJDIR)/integration-test/$(1)
endef
$(OBJDIR)/integration-test/automatic.txt: $(LOCAL_MKS)
$(MKDIR) "$(OBJDIR)/integration-test"
$(RM) $@
@$(foreach test,$(RUN_INTEGRATION_TEST),echo $(test)>>$@;)

Copilot uses AI. Check for mistakes.
Comment on lines +535 to +539
max_accounts = 1_300_000_000

# File size of the account database file (see [paths.accounts]).
# The entire account database is physically preallocated.
file_size_gib = 16
file_size_gib = 800
Copy link

Copilot AI Feb 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These new defaults (max_accounts = 1_300_000_000, file_size_gib = 800) drastically increase baseline resource requirements (index RAM + preallocated accounts DB). Because src/app/firedancer/config/devnet.toml does not override [accounts], running against devnet will now inherit these mainnet-scale defaults and likely fail on typical dev machines. Consider keeping smaller defaults and overriding only in mainnet configs, or add a devnet-specific [accounts] override with devnet-appropriate sizing.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants